-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: topic-email-management #333
Automated Test: topic-email-management #333
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a topic unsubscribe feature enabling users to stop receiving notifications for specific topics. Changes span frontend Ember.js routes, controllers, views, and templates; backend Rails routes and controller actions; email system integration with unsubscribe links; internationalization strings; and styling. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Frontend Route
participant Backend as TopicsController
participant Model as Topic Model
participant EmailSystem as Email System
User->>Frontend: Visit /t/:slug/:id/unsubscribe
Frontend->>Backend: GET unsubscribe action
Backend->>Model: Load topic & notification level
Model-->>Backend: Topic loaded
Backend->>Model: Update notification_level to :muted
Model-->>Backend: Changes saved
Backend-->>Frontend: Render unsubscribe template
Frontend-->>User: Display unsubscribe confirmation UI
Note over EmailSystem: Subsequent emails include unsubscribe link
EmailSystem->>EmailSystem: Use topic.unsubscribe_url in email footer
EmailSystem-->>User: Send notification with unsubscribe option
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/models/topic_user.rb (1)
120-125:⚠️ Potential issue | 🟠 MajorPre-existing bug on line 122:
user_idis assignedtopicinstead ofuser.Line 122 reads
user_id = user.is_a?(User) ? user.id : topic— whenuseris already an integer ID, this incorrectly assigns thetopicvariable touser_id. It should be: user. This is not introduced by this PR but is adjacent to the changed line 125 and could cause data corruption intrack_visit!.🐛 Suggested fix
- user_id = user.is_a?(User) ? user.id : topic + user_id = user.is_a?(User) ? user.id : userapp/assets/javascripts/discourse/routes/app-route-map.js.es6 (1)
8-13:⚠️ Potential issue | 🟠 MajorRoute conflict:
topicUnsubscribecannot be reached due totopic.fromParamsNearshadowing.In Ember.js, the URL
/t/:slug/:id/unsubscribematches thetopicresource first, then attempts to match the remaining/unsubscribesegment against its child routes. SincefromParamsNearhas the pattern/:nearPost, it will match/unsubscribewithnearPost="unsubscribe", preventing the top-leveltopicUnsubscriberoute from ever being reached.To fix this, either:
- Move
topicUnsubscribeas a child of thetopicresource (preferred for consistency with topic-related routes), or- Use a more specific static path pattern instead of a dynamic segment for
fromParamsNearto avoid the conflict.app/controllers/topics_controller.rb (1)
8-28:⚠️ Potential issue | 🟡 MinorAdd trailing comma to satisfy style check.
RuboCop flags the missing trailing comma in the multiline array.
Proposed fix
- :unsubscribe] + :unsubscribe,]
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6`:
- Around line 5-7: Rename the computed property stopNotificiationsText to the
correct stopNotificationsText in the controller (update the function name and
its .property declaration) and update any template bindings that reference the
misspelled name (e.g., the unsubscribe template's use of stopNotificiationsText)
to use stopNotificationsText so the property name matches English and remains
consistent across controller and template.
In `@app/controllers/topics_controller.rb`:
- Around line 98-115: The unsubscribe action currently mutates TopicUser state
on a GET which is unsafe; change it to only accept non-GET side-effectful
requests by updating the controller to require a POST/DELETE (or verify a signed
token) before changing TopicUser.notification_level: keep TopicView and the
lookup (TopicView.new and TopicUser.find_by) but gate the mutation behind a
request.post?/request.delete? check or validate a signed param/token, and ensure
CSRF protection is enabled; after the mutation still call tu.save! and
perform_show_response for the POST flow, and for GET requests only perform a
safe redirect to `@topic_view.topic.unsubscribe_url` without changing tu.
- Around line 105-113: Currently the code assumes TopicUser.find_by returns a
record and dereferences tu.notification_level, which raises NoMethodError for
users without a TopicUser; replace the find_by call with
TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id:
params[:topic_id]) (or use a TopicUser.change helper) to ensure tu is present,
then compute the target level using TopicUser.notification_levels[:regular] and
[:muted] and assign tu.notification_level accordingly before calling tu.save!;
update the method to use the tu variable and ensure it handles both new and
existing records.
In `@app/views/email/notification.html.erb`:
- Line 10: The h4 element uses an incorrect class attribute value
"class='.previous-discussion'" so the leading dot prevents CSS matching; update
the attribute on the h4 (the element with the text t
"user_notifications.previous_discussion") to remove the leading period (use
"previous-discussion" as the class value) so the class matches your stylesheet.
- Around line 5-9: The footer's respond_instructions are being duplicated
because the div "<div class='footer'>%{respond_instructions}</div>" is rendered
inside the context_posts conditional and again elsewhere; remove that inner
footer from the context_posts block (or change the conditional to render
respond_instructions only in the outer/footer area) so "%{respond_instructions}"
appears exactly once in app/views/email/notification.html.erb.
🧹 Nitpick comments (5)
app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 (3)
4-9: No error handling forloadTopicView— user sees a generic error on failure.If the topic doesn't exist or the network request fails, the rejected promise propagates to the default Ember route error handler. For an unsubscribe page (often reached from email links), showing a friendly error message is important.
💡 Suggested improvement
model(params) { const topic = this.store.createRecord("topic", { id: params.id }); return PostStream.loadTopicView(params.id).then(json => { topic.updateFromJson(json); return topic; + }).catch(() => { + return Ember.RSVP.reject({ not_found: true }); }); },Alternatively, implement an
erroraction in the route to render a user-friendly message for unsubscribe failures.
12-15:afterModelassumestopic.detailsis populated — add a guard.If
updateFromJsonfails to populatedetails(e.g., unexpected JSON shape), this will throw. A simple guard avoids a runtime error on the unsubscribe page.🛡️ Defensive check
afterModel(topic) { // hide the notification reason text - topic.set("details.notificationReasonText", null); + if (topic.get("details")) { + topic.set("details.notificationReasonText", null); + } },
1-3:Discourse.Routeis used as a global whilePostStreamis explicitly imported.For consistency with the
import PostStreamon Line 1, consider importing the Route base class as well instead of relying on theDiscourseglobal. This is a minor consistency nit in the context of a legacy codebase.app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (1)
11-16: Partial ES6 modernization:self = thispattern can be eliminated with arrow functions.The
var→constupgrade is good, butconst self = this(Line 11) is still needed only because Line 23 usesfunction(). Converting to an arrow function would complete the modernization and remove theselfalias.♻️ Suggested change
- const self = this, - topic = this.modelFor('topic'), + const topic = this.modelFor('topic'), postStream = topic.get('postStream'), topicController = this.controllerFor('topic'), topicProgressController = this.controllerFor('topic-progress'), composerController = this.controllerFor('composer'); ... - postStream.refresh(params).then(function () { + postStream.refresh(params).then(() => { ... Ember.run.scheduleOnce('afterRender', function() { - self.appEvents.trigger('post:highlight', closest); + this.appEvents.trigger('post:highlight', closest); });Also applies to: 23-23
app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 (1)
1-3: Minor version correction needed in deprecation timeline: ObjectController was deprecated in Ember 1.11 (not 1.13) and removed in Ember 2.0.Note that this codebase uses a custom
ObjectControllerwrapper defined inapp/assets/javascripts/discourse/controllers/object.js.es6which extendsEmber.ObjectController. If modernizing, replace withEmber.Controllerand explicitly referencemodelproperties rather than relying on proxying behavior. This pattern is used extensively across 40+ controllers in the codebase, so refactoring should be systematic if undertaken.
| stopNotificiationsText: function() { | ||
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | ||
| }.property("model.fancyTitle"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
rg -n 'stopNotifi' --type-list | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 477
🏁 Script executed:
rg -in 'stopNotifi' app/ --type-listRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 477
🏁 Script executed:
# Search for the property name in both JS and template files
find app -type f \( -name "*.es6" -o -name "*.js" -o -name "*.hbs" \) | xargs rg -l 'stopNotifi' 2>/dev/nullRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 222
🏁 Script executed:
# More targeted search - look for the exact property and its usage
rg -n 'stopNotifi' app/assets/javascripts/ app/assets/templates/ 2>/dev/null | head -50Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 298
🏁 Script executed:
# Search for the correctly spelled variant
rg -n 'stopNotifications' app/ --type-list 2>/dev/nullRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
🏁 Script executed:
# Check if there are any other references or usages of this property
rg -n 'unsubscribe' app/assets/javascripts/discourse/controllers/ app/assets/javascripts/discourse/templates/ 2>/dev/nullRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 950
🏁 Script executed:
# Look for any imports or references to topic-unsubscribe controller elsewhere
rg -n 'topic-unsubscribe' app/ 2>/dev/nullRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 259
Typo in computed property name: stopNotificiationsText → stopNotificationsText.
The property name has an extra i — Notificiations instead of Notifications. Although the template also uses the same misspelled name (so the binding works), this creates confusion for any future developer and doesn't match the actual English word. Fix both the property definition and template reference.
✏️ Proposed fix
- stopNotificiationsText: function() {
+ stopNotificationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),Also update app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs line 3:
- {{{stopNotificiationsText}}}
+ {{{stopNotificationsText}}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stopNotificiationsText: function() { | |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | |
| }.property("model.fancyTitle"), | |
| stopNotificationsText: function() { | |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | |
| }.property("model.fancyTitle"), |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6` around
lines 5 - 7, Rename the computed property stopNotificiationsText to the correct
stopNotificationsText in the controller (update the function name and its
.property declaration) and update any template bindings that reference the
misspelled name (e.g., the unsubscribe template's use of stopNotificiationsText)
to use stopNotificationsText so the property name matches English and remains
consistent across controller and template.
| def unsubscribe | ||
| @topic_view = TopicView.new(params[:topic_id], current_user) | ||
|
|
||
| if slugs_do_not_match || (!request.format.json? && params[:slug].blank?) | ||
| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 | ||
| end | ||
|
|
||
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | ||
|
|
||
| if tu.notification_level > TopicUser.notification_levels[:regular] | ||
| tu.notification_level = TopicUser.notification_levels[:regular] | ||
| else | ||
| tu.notification_level = TopicUser.notification_levels[:muted] | ||
| end | ||
|
|
||
| tu.save! | ||
|
|
||
| perform_show_response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid state changes via GET in unsubscribe.
This action mutates notification settings on a GET request, which is CSRF‑prone for logged‑in users. Consider switching to POST with CSRF protection or using a signed token in the URL to permit a safe one‑click flow.
🧰 Tools
🪛 RuboCop (1.84.0)
[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]
(Metrics/AbcSize)
🤖 Prompt for AI Agents
In `@app/controllers/topics_controller.rb` around lines 98 - 115, The unsubscribe
action currently mutates TopicUser state on a GET which is unsafe; change it to
only accept non-GET side-effectful requests by updating the controller to
require a POST/DELETE (or verify a signed token) before changing
TopicUser.notification_level: keep TopicView and the lookup (TopicView.new and
TopicUser.find_by) but gate the mutation behind a request.post?/request.delete?
check or validate a signed param/token, and ensure CSRF protection is enabled;
after the mutation still call tu.save! and perform_show_response for the POST
flow, and for GET requests only perform a safe redirect to
`@topic_view.topic.unsubscribe_url` without changing tu.
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | ||
|
|
||
| if tu.notification_level > TopicUser.notification_levels[:regular] | ||
| tu.notification_level = TopicUser.notification_levels[:regular] | ||
| else | ||
| tu.notification_level = TopicUser.notification_levels[:muted] | ||
| end | ||
|
|
||
| tu.save! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle missing TopicUser rows before accessing notification_level.
TopicUser.find_by can return nil, causing a NoMethodError for users who never had a TopicUser row. Use find_or_initialize_by or TopicUser.change with a computed level so unsubscribe works for all users (and this also helps keep the method’s ABC size down).
Proposed fix
- tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
-
- if tu.notification_level > TopicUser.notification_levels[:regular]
- tu.notification_level = TopicUser.notification_levels[:regular]
- else
- tu.notification_level = TopicUser.notification_levels[:muted]
- end
-
- tu.save!
+ tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id])
+ current_level = tu.notification_level || TopicUser.notification_levels[:regular]
+ new_level =
+ if current_level > TopicUser.notification_levels[:regular]
+ TopicUser.notification_levels[:regular]
+ else
+ TopicUser.notification_levels[:muted]
+ end
+
+ TopicUser.change(current_user, params[:topic_id], notification_level: new_level)🤖 Prompt for AI Agents
In `@app/controllers/topics_controller.rb` around lines 105 - 113, Currently the
code assumes TopicUser.find_by returns a record and dereferences
tu.notification_level, which raises NoMethodError for users without a TopicUser;
replace the find_by call with TopicUser.find_or_initialize_by(user_id:
current_user.id, topic_id: params[:topic_id]) (or use a TopicUser.change helper)
to ensure tu is present, then compute the target level using
TopicUser.notification_levels[:regular] and [:muted] and assign
tu.notification_level accordingly before calling tu.save!; update the method to
use the tu variable and ensure it handles both new and existing records.
| <% if context_posts.present? %> | ||
| <div class='footer'>%{respond_instructions}</div> | ||
|
|
||
| <hr> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid duplicate respond instructions when context posts exist.
%{respond_instructions} is rendered inside the context_posts block and again in the footer, causing duplicate text in emails with context. Remove the inner footer.
Proposed fix
- <% if context_posts.present? %>
- <div class='footer'>%{respond_instructions}</div>
-
- <hr>
+ <% if context_posts.present? %>
+ <hr>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <% if context_posts.present? %> | |
| <div class='footer'>%{respond_instructions}</div> | |
| <hr> | |
| <% if context_posts.present? %> | |
| <hr> | |
🤖 Prompt for AI Agents
In `@app/views/email/notification.html.erb` around lines 5 - 9, The footer's
respond_instructions are being duplicated because the div "<div
class='footer'>%{respond_instructions}</div>" is rendered inside the
context_posts conditional and again elsewhere; remove that inner footer from the
context_posts block (or change the conditional to render respond_instructions
only in the outer/footer area) so "%{respond_instructions}" appears exactly once
in app/views/email/notification.html.erb.
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render :partial => 'email/post', :locals => {:post => p} %> | ||
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix class attribute typo for the previous‑discussion header.
class='.previous-discussion' includes a leading dot, so the class won’t match CSS selectors.
Proposed fix
- <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
+ <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | |
| <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
🤖 Prompt for AI Agents
In `@app/views/email/notification.html.erb` at line 10, The h4 element uses an
incorrect class attribute value "class='.previous-discussion'" so the leading
dot prevents CSS matching; update the attribute on the h4 (the element with the
text t "user_notifications.previous_discussion") to remove the leading period
(use "previous-discussion" as the class value) so the class matches your
stylesheet.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Localization